Skip to content

Percent-decode local image hrefs - #1121

Closed
teddytennant wants to merge 1 commit into
linebender:mainfrom
teddytennant:fix/percent-encoded-image-href
Closed

Percent-decode local image hrefs#1121
teddytennant wants to merge 1 commit into
linebender:mainfrom
teddytennant:fix/percent-encoded-image-href

Conversation

@teddytennant

Copy link
Copy Markdown

Fixes #1073.

The bug

An <image> href is an IRI, so SVG editors percent-encode characters that
can't appear in one. Inkscape, for example, writes a local file named
images/细节3-mine.png as:

<image href="images/%E7%BB%86%E8%8A%823-mine.png" .../>

ImageHrefResolver::default_string_resolver passed that string straight to
Options::get_abs_path and then to Path::exists, so it looked for a file
literally named %E7%BB%86%E8%8A%823-mine.png. That file doesn't exist, and the
image was silently dropped:

Warning (in usvg::parser::image:110): 'images/%E7%BB%86%E8%8A%823-mine.png' is not a path to an image.

Reproducer (from the issue):

repro/
  encoded.svg      <image href="images/%E7%BB%86%E8%8A%823-mine.png" .../>
  images/细节3-mine.png
resvg --resources-dir ./repro ./repro/encoded.svg out.png

The same SVG with the decoded file name in the href renders fine.

The fix

The default string resolver now percent-decodes the href and retries, using a
small local decoder (no new dependency).

The raw path is still checked first and the decoded path is only used as a
fallback, so a file whose name genuinely contains a percent sign — including
one that happens to look like a valid escape sequence, e.g. %41.svg — keeps
resolving exactly as before. Nothing that works today changes behaviour; this
only adds a second attempt for hrefs that currently fail. Invalid escape
sequences are left as-is, and if the decoded bytes aren't valid UTF-8 the
original path is kept.

This is fixed in the resolver rather than in the parser because that's where the
href is turned into a filesystem path, and it keeps custom
ImageHrefResolvers free to do their own thing.

Deliberately out of scope

  • data: URLs — they're handled by resolve_data and never touch the filesystem.
  • fontdb/resources_dir path handling in general.
  • Interpreting the href as a full URL (scheme, authority, query, + as space).
    usvg doesn't resolve URLs, and treating + as a space would break existing
    file names.

Verification

New tests in crates/usvg/tests/parser.rs. Before the change:

running 2 tests
test percent_encoded_image_href ... FAILED
test image_href_with_literal_percent ... ok

---- percent_encoded_image_href stdout ----
thread 'percent_encoded_image_href' panicked at crates/usvg/tests/parser.rs:617:5:
assertion failed: is_image_resolved(&dir, "images/%E7%BB%86%E8%8A%823-mine.svg")

test result: FAILED. 1 passed; 1 failed; 0 ignored; 0 measured; 27 filtered out

After:

test result: ok. 29 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

cargo test --all --release is green: 1730 render tests, 29 usvg parser tests,
31 usvg writer tests, 2 resvg unit tests, 1 doctest. cargo fmt --all --check,
.github/copyright.sh and typos are clean (the two typos hits on main
oppen in the changelog and planed in parser/filter.rs — are pre-existing
and untouched). Also built with an older toolchain to check nothing here needs a
recent compiler.

An <image> href is an IRI, so SVG editors like Inkscape store non-ASCII
file names percent-encoded. The default string resolver passed the href
to the filesystem verbatim, so such images were never found and silently
dropped with an 'is not a path to an image' warning.

The raw path is still checked first, so file names containing a literal
percent sign keep working.
@RazrFalcon

Copy link
Copy Markdown
Collaborator

Thanks, but that's not how it should be implemented.

The fix should land into svgtypes first. And then we need a pair of SVG + PNG for testing.

@teddytennant

Copy link
Copy Markdown
Author

Makes sense, closing this. Hand-rolling the decoding in the resolver was the wrong layer, and it only helps callers who use the default resolver anyway.

I can put it in svgtypes instead and come back here with the SVG and PNG pair once that lands, if you want it.

@RazrFalcon

Copy link
Copy Markdown
Collaborator

Yep. You can add SVG + PNG pair together with svgtypes bump.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Percent-encoded non-ASCII image hrefs are not resolved against resources-dir

2 participants